Skip to content

fix(envoy-client): dont abort drain after 20s#5043

Draft
MasterPtato wants to merge 1 commit into
05-11-fix_wasm_point_wasm-pack_build_to_new_wasm-bindgen_repofrom
05-11-fix_envoy-client_dont_abort_drain_after_20s
Draft

fix(envoy-client): dont abort drain after 20s#5043
MasterPtato wants to merge 1 commit into
05-11-fix_wasm_point_wasm-pack_build_to_new_wasm-bindgen_repofrom
05-11-fix_envoy-client_dont_abort_drain_after_20s

Conversation

@MasterPtato
Copy link
Copy Markdown
Contributor

Description

Please include a summary of the changes and the related issue. Please also include relevant motivation and context.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes.

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@MasterPtato MasterPtato changed the base branch from counter-latency/require-engine-ping-health to graphite-base/5043 May 12, 2026 01:21
@MasterPtato MasterPtato force-pushed the 05-11-fix_envoy-client_dont_abort_drain_after_20s branch from 2ad5377 to b28585d Compare May 12, 2026 01:21
@MasterPtato MasterPtato changed the base branch from graphite-base/5043 to 05-11-fix_wasm_point_wasm-pack_build_to_new_wasm-bindgen_repo May 12, 2026 01:21
@claude
Copy link
Copy Markdown
Contributor

claude Bot commented May 12, 2026

PR #5043 Review: fix(envoy-client): dont abort drain after 20s

Critical Issues

1. Undefined Variable Usage — BLOCKER

File: rivetkit-rust/packages/rivetkit-core/src/registry/mod.rs

The diff removes the SHUTDOWN_DRAIN_TIMEOUT constant but it appears to still be referenced downstream. Combined with an unused stop_threshold variable that was presumably meant to replace it, this looks like incomplete work that will fail to compile.

Fix required: Either use stop_threshold where SHUTDOWN_DRAIN_TIMEOUT was, or remove stop_threshold and the TODO comment if the dynamic approach isn't ready for this PR.

Verify with: cargo check -p rivetkit-core

2. Unused Variable

File: rivetkit-rust/packages/rivetkit-core/src/registry/mod.rs

stop_threshold is defined but never used in the timeout call. The TODO comment says "Read threshold from protocol metadata, fall back to 30 min" but the implementation doesn't follow through — the dynamic value is computed but then discarded.


Code Quality

3. Kitchen Sink Mode Module (mode.ts)

The new mode.ts module cleanly separates three deployment modes (serverful, serverless, serverless-local). Logic is clear and well-documented. The Docker base image change from node:22-slim to node:22-trixie-slim (Debian 13) is reasonable.

4. Config Changes (pegboard.rs)

Numeric literal style (5 * 60 * 1000 for time values) is more readable than raw underscored integers and matches existing patterns in the file.


Test Coverage

No tests are included for the drain timeout behavior change. Before merging:

  • Confirm compilation succeeds end-to-end
  • Test shutdown drain behavior with the corrected timeout
  • Test all three kitchen sink modes

Summary

Status: Needs fixes before merge

Required:

  1. Resolve the SHUTDOWN_DRAIN_TIMEOUT/stop_threshold inconsistency — either wire up the dynamic value or revert to a constant until the dynamic approach is ready
  2. Confirm cargo check -p rivetkit-core passes

Nice to have:

  • Add a test for shutdown drain with the new timeout logic
  • Document where actor_stop_threshold comes from in the protocol metadata

@MasterPtato MasterPtato force-pushed the 05-11-fix_envoy-client_dont_abort_drain_after_20s branch from b28585d to f2f82a8 Compare May 14, 2026 17:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant